Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release/7.0] fix TLS resume with client certificates on Linux #81795

Merged
merged 1 commit into from
Feb 8, 2023
Merged

[release/7.0] fix TLS resume with client certificates on Linux #81795

merged 1 commit into from
Feb 8, 2023

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Feb 7, 2023

partial backport of #79898
fixes #79869

Customer Impact

because of the TLS resume, customer can see unexpected dependencies and incorrect behavior when using client certificates on Linux. The TLS cache in in process memory so the impact is limited.

Testing

New tests were added to cover the scenarios.
The tests depend on IsMutuallyAuthenticated property that may be incorrect because of #65563.
Since we do not port fix for that I made tests Linux specific for the release branch. (there is no resume on macOS)
(the tests added in main also discover gaps so the product change #79898 had more fixes for Window)

Regression

yes, new in 7.0

Risk Low

We did not do TLS resume at all prior 7.0 on Linux. This change brings back the behavior in one more case we missed.

@wfurt wfurt added Servicing-consider Issue for next servicing release review area-System.Net.Security os-linux Linux OS (any supported distro) labels Feb 7, 2023
@wfurt wfurt requested a review from rzikm February 7, 2023 23:17
@wfurt wfurt self-assigned this Feb 7, 2023
@ghost
Copy link

ghost commented Feb 7, 2023

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

partial backport of #79898
fixes #79869

Customer Impact

because of the TLS resume, customer can see unexpected dependencies and incorrect behavior when using client certificates on Linux. The TLS cache in in process memory so the impact is limited.

Testing

New tests were added to cover the scenarios.
The tests depend on IsMutuallyAuthenticated property that may be incorrect because of #65563.
Since we do not port fix for that I made tests Linux specific for the release branch. (there is no resume on macOS)
(the tests added in main also discover gaps so the product change #79898 had more fixes for Window)

Regression

yes, new in 7.0

Risk Low

We did not do TLS resume at all prior 7.0 on Linux. This change brings back the behavior in one more case we missed.

Author: wfurt
Assignees: wfurt
Labels:

Servicing-consider, area-System.Net.Security, os-linux
Milestone: -

@wfurt wfurt changed the title [release/7.0] fix TLS resume with client certificates [release/7.0] fix TLS resume with client certificates on Linux Feb 7, 2023
@karelz karelz added this to the 7.0.x milestone Feb 8, 2023
@karelz
Copy link
Member

karelz commented Feb 8, 2023

Tactics approval via email by @SteveMCarroll on Wed 2/8.
Marking it servicing-approved.

@karelz karelz added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 8, 2023
@karelz
Copy link
Member

karelz commented Feb 8, 2023

@rzikm can you please help look at the test failures and link them to known issues? @carlossanlop will have easier time to merge it later. Thank you!

@rzikm
Copy link
Member

rzikm commented Feb 8, 2023

First one is Helix API does not contain an entry for Windows.10.Arm64.Open, did we remove this queue from CI? @MattGal Do you have some idea what is going on?

The other one is Microsoft.VisualBasic.Core.Tests, which looks like #81544

@karelz
Copy link
Member

karelz commented Feb 8, 2023

Looks like we are clean on test results, so we should be ready to merge when suitable - @carlossanlop let us know if we missed anything. Thanks!

@MattGal
Copy link
Member

MattGal commented Feb 8, 2023

First one is Helix API does not contain an entry for Windows.10.Arm64.Open, did we remove this queue from CI? @MattGal Do you have some idea what is going on?

The other one is Microsoft.VisualBasic.Core.Tests, which looks like #81544

Yes, aside from the warnings that appear in logging (example):

##[warning]SENDHELIXJOB(0,0): warning : Helix queue windows.10.arm64.open was set for estimated removal date of 2023-02-01. In most cases the queue will be removed permanently due to end-of-life; please contact dnceng for any questions or concerns, and we can help you decide how to proceed and discuss other options.

there is also a DL called "dncpartners" where this removal was communicated many weeks out from the actual removal. We've moved to Azure-based VMs, please use windows.11.arm64 and windows.11.arm64.open instead.

@carlossanlop carlossanlop modified the milestones: 7.0.x, 7.0.4 Feb 8, 2023
@carlossanlop
Copy link
Member

Approved by Tactics for 7.0.4.
Signed off by area owner.
CI failures are unrelated - I merged the fix for the update to Win10 helix queues yesterday, but after this PR was created (hence the failure).
No OOB changes needed for System.Net.Security.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit 6cad173 into dotnet:release/7.0 Feb 8, 2023
@wfurt
Copy link
Member Author

wfurt commented Feb 8, 2023

thanks everyone.

@wfurt wfurt mentioned this pull request Feb 8, 2023
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rzikm rzikm rzikm approved these changes

Assignees

@wfurt wfurt

Labels
area-System.Net.Security os-linux Linux OS (any supported distro) Servicing-approved Approved for servicing release
Projects
None yet
Milestone
7.0.4
Development

Successfully merging this pull request may close these issues.
5 participants
@wfurt @karelz @rzikm @MattGal @carlossanlop